dynamic_modules: HTTP filter config implementation #37070
Conversation
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
cc @marc-barry |
|
sorry for the slow progress for the last few weeks - i've been swamped by other works, now I managed to allocate some cycles on this one. The plan is after this PR, the per-stream event hooks will be implemented and then module->envoy call backs (e.g. getting headers etc) will be added. Feels like we will soon be able to provide basic functionality. |
|
/retest |
|
As a side note, Linkedin's infra team has extensively tested my PoC impl, and according to their evaluation, it's working pretty great and they observed almost zero overhead compared with the native C++ impl, which sounds awesome. The work here is and subsequent PRs will be based on their feedback as well as the original PoC implementation. |
| typedef const void* // NOLINT(modernize-use-using) | ||
| envoy_dynamic_module_type_http_filter_config_envoy_ptr; |
There was a problem hiding this comment.
I'm not sure I understand the purpose of passing void* here if the user has to have the actual C++ definition. Is this just to avoid any C++ leaking into the ABI itself?
There was a problem hiding this comment.
That's a valid question. The users (modules) do not need to understand the C++ definition, but instead users can just call the corresponding ABI functions with this opaque pointers. For example, one notable example about filter-config level ABI function is "creating metrics". Then possible function signature for that ABI would look like
envoy_dynamic_module_type_counter_metric_envoy_ptr envoy_dynamic_module_define_counter_metric(
envoy_dynamic_module_type_http_filter_config_envoy_ptr envoy_filter_config_ptr,
const char* name,
int name_length,
);where the module->Envoy ABI function has this opaque pointer as a first argument. Then on the Envoy side, we can provide the functionality
envoy_dynamic_module_type_counter_metric_envoy_ptr envoy_dynamic_module_define_counter_metric(
envoy_dynamic_module_type_http_filter_config_envoy_ptr envoy_filter_config_ptr,
const char* name,
int name_length,
) {
const std::string_view name_view(static_cast<const char*>(name), name_length);
auto scope = static_cast<DynamicModuleHttpFilterConfig*>(envoy_filter_config_ptr)->stat_scope_;
Stats::StatNameManagedStorage storage(name_view, scope->symbolTable());
Stats::StatName stat_name = storage.statName();
Stats::Counter& counter = scope->counterFromStatName(stat_name);
return &counter;
}without leaking the implementation details of this envoy side "HttpFilterConfig" class into dynamic modules.
Apologies this is difficult to see the rationale behind this pointer passing without the concrete impl example like this. Later when I add these ABI functions, then we will be able to see the concrete picture more clearly. Hope this helps!
| /// This is useful to perform any process-wide initialization that the dynamic module needs. | ||
| /// The first argument has [`ProgramInitFunction`] type, and it is called when the dynamic module is loaded. | ||
| /// | ||
| /// The second argument has [`NewHttpFilterConfigFunction`] type, and it is called when the new HTTP filter configuration is created. |
There was a problem hiding this comment.
nit: can you setup in some future PR rustfmt and make sure it runs in CI? This config https://github.com/bitdriftlabs/shared-core/blob/main/rustfmt.toml will make it mostly look like Envoy code style. For now you can run it manually.
There was a problem hiding this comment.
sure, it's already enabled in this PR with the default config
envoy/test/extensions/dynamic_modules/BUILD
Lines 78 to 90 in 7ecd263
so i will follow up with the TOML config after this
and add the unit tests around them. In addition, this replaces global static mut with LazyLock Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
i am surprised that ASAN works for Rust SDK tests as well (pure Rust codes) without any build tweaks, feeling good |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
looks like the exact issue is rust-lang/rust#39608 as I verified that empty .rs file with empty tests fails with sanitizer (meaning test harness itself is being detected by TSAN) |
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
|
/retest |
Commit Message: dynamic_modules: enables rustfmt.toml Additional Description: This enables the root configuration for rustfmt as a follow up on #37070 (comment). Only formatting is done in this commit. There's no change in its code. Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Commit Message: dynamic_modules: HTTP filter config implementation
Additional Description:
This expands the ABI for HTTP filter configurations. Especially this adds two
event hooks coupled with the life cycle of HTTP filter config handled in the main
thread.
The key idea is to do the direct pointer (context) passing between the boundary;
This allows us to avoid maintaining IDs and global mapping state, which makes it
easier to test as well as it has benefit in terms of performance. E.g. there's no
need to look up "contexts" on each event hook entry.
The next follow-up PR will add per-stream event hooks (filter implementation).
After the event hooks are done, module->Envoy functions will be added (e.g.
accessing headers, etc.)
Risk Level: low
Testing: done
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]